[GLUTEN-10056][FLINK] Support function split_index#10057
[GLUTEN-10056][FLINK] Support function split_index#10057shuai-xu merged 12 commits intoapache:mainfrom
Conversation
| public boolean isSupported(RexCall callNode, RexConversionContext context) { | ||
| if (!TypeUtils.isStringType(RexNodeConverter.toType(callNode.getType()))) { | ||
| return false; | ||
| } | ||
| List<RexNode> operands = callNode.getOperands(); | ||
| if (operands.size() != 3) { | ||
| return false; | ||
| } | ||
| RexNode firstOp = operands.get(0); | ||
| RexNode secondOp = operands.get(1); | ||
| RexNode thirdOp = operands.get(2); | ||
| if (!TypeUtils.isStringType(RexNodeConverter.toType(firstOp.getType())) | ||
| || !TypeUtils.isStringType(RexNodeConverter.toType(secondOp.getType())) | ||
| || !TypeUtils.isIntegerType(RexNodeConverter.toType(thirdOp.getType()))) { | ||
| return false; | ||
| } | ||
| if (!(secondOp instanceof RexLiteral) || !(thirdOp instanceof RexLiteral)) { | ||
| return false; | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
I think we could return true here without check, since it's the only convertor for split_index. The velox backend will do some validation check.
PHILO-HE
changed the title
shuai-xu
left a comment
shuai-xu
left a comment
There was a problem hiding this comment.
Which nexmark query use this function?
| public TypedExpr toTypedExpr(RexCall callNode, RexConversionContext context) { | ||
| List<TypedExpr> params = getParams(callNode, context); | ||
| ConstantTypedExpr indexExpr = (ConstantTypedExpr) params.get(params.size() - 1); | ||
| if (TypeUtils.isIntegerType(indexExpr.getReturnType())) { |
There was a problem hiding this comment.
Why need this transform? if it's not int, need to check it valid ?
There was a problem hiding this comment.
The corresponding function in velox is split_part, and the last parameter's type is int64_t, but in flink split_index function, it is integer type, so we need this transform
There was a problem hiding this comment.
please add comments for it.
| @Test | ||
| void testSplitIndex() { | ||
| List<Row> rows = | ||
| Arrays.asList( |
There was a problem hiding this comment.
Is there any corner case need to test to verify the result same with flink?
There was a problem hiding this comment.
we can cover this by use ut in flink ScalaFunctionsTest#testSplitIndex
There was a problem hiding this comment.
seems there are many null related tests in flink not covered by this test.
There was a problem hiding this comment.
and others such as index is invalid.
There was a problem hiding this comment.
I have copyed some related flink tests here related to the index invalid, input has nulls, or delimiter is defined by ascii, but others tests e.g. index parameter is null or delimiter parameter is null, can not run here. as we can not pass the the null constant parameter value to velox. Or we should introduce a Variant to pass null constant.
nexmark q22 need this |
| public TypedExpr toTypedExpr(RexCall callNode, RexConversionContext context) { | ||
| List<TypedExpr> params = getParams(callNode, context); | ||
| ConstantTypedExpr indexExpr = (ConstantTypedExpr) params.get(params.size() - 1); | ||
| if (TypeUtils.isIntegerType(indexExpr.getReturnType())) { |
There was a problem hiding this comment.
please add comments for it.
| @Test | ||
| void testSplitIndex() { | ||
| List<Row> rows = | ||
| Arrays.asList( |
There was a problem hiding this comment.
seems there are many null related tests in flink not covered by this test.
| @Test | ||
| void testSplitIndex() { | ||
| List<Row> rows = | ||
| Arrays.asList( |
There was a problem hiding this comment.
and others such as index is invalid.
3 participants
What changes were proposed in this pull request?
(Please fill in changes proposed in this fix)
(Fixes: #10056)
depends on pr: bigo-sg/velox#8
How was this patch tested?
UT